-
-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Discourage SolidusStarterFrontend from being run as an engine #167
Conversation
d632671
to
50b84e3
Compare
50b84e3
to
de6a548
Compare
de6a548
to
cfc6371
Compare
```sh | ||
# config/initializers/solidus_starter_frontend.rb | ||
ENV['SOLIDUS_STARTER_FRONTEND_ALLOW_AS_ENGINE'] = 'true' | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if there's a better way to do this. Setting SOLIDUS_STARTER_FRONTEND_ALLOW_AS_ENGINE
in the terminal doesn't seem to work with the bundle exec rails generate solidus:install
and bundle exec rails generate solidus:auth:install
calls in the next step.
docs/development.md
Outdated
@@ -42,6 +52,82 @@ Use Ctrl-C to stop | |||
|
|||
Default username and password for admin are: `[email protected]` and `test123`. | |||
|
|||
### Running the extension as an engine in a Rails app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in the video, one option is to create a script to automatically run these commands. Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I don't fully understand why we need to provide two different development options. Shouldn't it be enough to have the sandbox load solidus_starter_frontend
as a gem? If we point to the local gem (one directory up), changes will also be reflected. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@waiting-for-dev The two development options have different goals:
- As a gem contributor, I want a easy way to test if a
solidus_starter_frontend
can generate a working frontend for a Rails app. - As a gem contributor, I want a easy way to preview changes I make to the app code of
solidus_starter_frontend
.
Previously, the sandbox script was both loading the gem and generating the frontend. In #166, I changed the sandbox script to fulfill only goal 1, based on the reasoning that the gem will be used only as a generator in actual usage.
Goal 2, on the other hand, is more of a nice-to-have for the gem contributor, since it just makes it easier for the contributor to preview changes they make to the app code.
We could change the sandbox script to load the gem instead of using it to generate the frontend. Perhaps I might be misunderstanding what a sandbox app is supposed to do, based on conventions we follow with other Solidus extensions?
Your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm yeah, it makes sense. Ideally, I'd like to confirm that the solidus_starter_frontend
executable can generate a working frontend in the integrated test suite. Still, if that's not possible, then I agree we need some way of at least doing it manually.
Perhaps I might be misunderstanding what a sandbox app is supposed to do, based on conventions we follow with other Solidus extensions?
What do you mean by the conventions we follow with other extensions? I haven't worked with Solidus extensions, so I'm unfamiliar. Isn't the sandbox app there picking changes made to the extension code? At least, it's what I do on the main Solidus repo, so I had the idea it was one of its purposes.
Anyway, yeah, probably we need both strategies, but I definitely would automate both of them. Maybe we could even provide a flag to bin/sandbox
so switch from one to the other, defaulting to the as-gem version as I think it'll be more common. However, if we take this direction, it'd be nice to be able to configure the application name so that generating it with another strategy doesn't forcefully means overwriting the previous one (we could even use always two different names, like sandbox_as_gem
and sandbox_as_exec
🙃 )
I'd like to ping @kennyadsl to hear what are his thoughts on this, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@waiting-for-dev I agree with defaulting to as-gem version since 1) this seems to be the more common way you'd want to use the sandbox, and 2) based on what you said, it's how the sandbox behaves in the main Solidus repo.
I also like the idea of have two different scripts/apps for sandbox_as_gem
and sandbox_as_exec
.
@@ -15,6 +15,8 @@ class Engine < Rails::Engine | |||
end | |||
|
|||
config.to_prepare do | |||
raise 'Error: SolidusStarterFrontend is not allowed to run as an engine' unless ENV['SOLIDUS_STARTER_FRONTEND_ALLOW_AS_ENGINE'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate your suggestions on improving the copy of this error message.
@@ -2,6 +2,7 @@ | |||
|
|||
# Configure Rails Environment | |||
ENV['RAILS_ENV'] = 'test' | |||
ENV['SOLIDUS_STARTER_FRONTEND_ALLOW_AS_ENGINE'] = 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed when running bundle exec rspec
.
@@ -1,6 +1,8 @@ | |||
# frozen_string_literal: true | |||
|
|||
require 'solidus_dev_support/rake_tasks' | |||
|
|||
ENV['SOLIDUS_STARTER_FRONTEND_ALLOW_AS_ENGINE'] = 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed when running bundle exec rake
.
docs/development.md
Outdated
#### 4) Install the frontend assets | ||
|
||
You will need to run `bundle exec rails g solidus_starter_frontend:install` to add the | ||
frontend assets to the existing vendored Solidus manifest files. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized now that instead of running this install command when running the gem as an engine, we could add the assets directly to app/views/spree/layouts/spree_application.html.erb
and then strip them from the layout when running the generator. Let me know what you think!
---- As a solidus_starter_frontend contributor I want the gem's sandbox script to generate the app with the solidus_starter_frontend gem as an engine So that the app would pick up changes I make to the app code Background ---------- Currently, the sandbox script both loads the gem and runs its generator. We want it to only the load the gem. Previous implementations ------------------------ * [Document how to run the extension as an engine in a Rails app](a4600e2) - This commit documents how to install solidus_starter_frontend as an engine. * Fix sandbox script #166 - This PR fixed the script to run `solidus_starter_frontend` as a generator instead of as a gem. Relevant links -------------- #167 (comment) - discussion on having two strategies for generating the sandbox app: one with the Starter FE running as an engine and other where Starter FE is generated into the app. Implementation -------------- * Point solidus gems in sandbox Gemfile to those in solidus_starter_frontend's Gemfile. * Fix: Install solidus using `solidus:install` instead of `spree:install`. * Fix: Pass `--auto-run-migrations` instead of `--auto-accept` to `solidus:auth:install`. * Remove `solidus_starter_frontend` call. * Call `bundle exec rails g solidus_starter_frontend:install`.
Goal ---- As a solidus_starter_frontend contributor I want the gem's sandbox script to generate the app with the solidus_starter_frontend gem as an engine So that the app would pick up changes I make to the app code Background ---------- Currently, the sandbox script both loads the gem and runs its generator. We want it to only the load the gem. Previous implementations ------------------------ * [Document how to run the extension as an engine in a Rails app](a4600e2) - This commit documents how to install solidus_starter_frontend as an engine. * Fix sandbox script #166 - This PR fixed the script to run `solidus_starter_frontend` as a generator instead of as a gem. Relevant links -------------- #167 (comment) - discussion on having two strategies for generating the sandbox app: one with the Starter FE running as an engine and other where Starter FE is generated into the app. Implementation -------------- * Point solidus gems in sandbox Gemfile to those in solidus_starter_frontend's Gemfile. * Fix: Install solidus using `solidus:install` instead of `spree:install`. * Fix: Pass `--auto-run-migrations` instead of `--auto-accept` to `solidus:auth:install`. * Remove `solidus_starter_frontend` call. * Call `bundle exec rails g solidus_starter_frontend:install`.
… app Goal ---- As a `solidus_starter_frontend` contributor I want an alternate sandbox script which copies Starter FE into the sandbox application So that I have an easy way to confirm if the Starter FE can generate a working frontend for a Rails app. Previous implementations ------------------------ * Fix sandbox script #166. Relevant links -------------- #167 (comment) - discussion on having two strategies for generating the sandbox app: one with the Starter FE running as an engine and other where Starter FE is generated into the app.
Goal ---- As a `solidus_starter_frontend` contributor I want an alternate sandbox script which copies Starter FE into the sandbox application So that I have an easy way to confirm if the Starter FE can generate a working frontend for a Rails app. Previous implementations ------------------------ * Fix sandbox script #166. Relevant links -------------- #167 (comment) - discussion on having two strategies for generating the sandbox app: one with the Starter FE running as an engine and other where Starter FE is generated into the app.
Goal ---- As a `solidus_starter_frontend` contributor I want an alternate sandbox script which copies Starter FE into the sandbox application So that I have an easy way to confirm if the Starter FE can generate a working frontend for a Rails app. Previous implementations ------------------------ * Fix sandbox script #166. Relevant links -------------- #167 (comment) - discussion on having two strategies for generating the sandbox app: one with the Starter FE running as an engine and other where Starter FE is generated into the app.
Goal ---- As a `solidus_starter_frontend` contributor I want an alternate sandbox script which copies Starter FE into the sandbox application So that I have an easy way to confirm if the Starter FE can generate a working frontend for a Rails app. Previous implementations ------------------------ * Fix sandbox script #166. Relevant links -------------- #167 (comment) - discussion on having two strategies for generating the sandbox app: one with the Starter FE running as an engine and other where Starter FE is generated into the app.
Dependencies
Depends on #166.
Motivation and Context
From https://www.notion.so/nebulab/Starter-FE-7f00e5d23f5841c1817beebfc8859d83:
Walkthrough and Demo
Demo: https://www.loom.com/share/a7be0f380b944cce9678796e6c88be12
Types of changes
Checklist: